fix: support polymodels with equal names but different roots (#961)#986
fix: support polymodels with equal names but different roots (#961)#986ventice11o wants to merge 1 commit intogoogleapis:mainfrom
Conversation
product-auto-label
bot
added
the
api: datastore
| names, like regular Model class names, must be globally unique. | ||
| have the same kind, but different class names. | ||
| """ | ||
| cls._kind_map[cls._class_name()] = cls |
There was a problem hiding this comment.
This is the reason why class names have to be globally unique, unfortunately. Different model classes will overwrite each other here if they have the same class name.
You can see this by adding the following to the end of your test below:
assert isinstance(CatCommand._kind_map["Cat"].bar, model.StringProperty)
assert not isinstance(CatAnimal._kind_map["Cat"].bar, model.StringProperty)Notice how on the second line, even though we're accessing it through CatAnimal, the model referenced by CatAnimal._kind_map["Cat"] is for a CatCommand.
There was a problem hiding this comment.
Yes, they are overwritten, but there's a code above it that uses its own dictionary using fully qualified names to look up for the real entity during model instance creation. So, yes, non-unique polymorphic classes, when accessed using _kind_map will be buggy, but why would a polymodel be looked up via global registry of top level classes? Polymodels provide their own registry and don't care what other registries think of them.
At least my legacy code with my fix doesn't experience any problems in my scenario as it did not in legacy ndb. And I use polymorphic models actively.
There was a problem hiding this comment.
Looks like I can override _class_name for PolyModel to return something like Animal.Cat and Command.Cat. In this case even _kind_map will keep its invariant.
3 participants
In the new version of NDB a breaking requirement/implementation has been added requiring all the polymodel leaves to have unique names. There is no technical limitation for this requirement either on client or server side. So, by the introduction of this requirement we make it unable for the clients with legacy data to retrieve their models written by
appengine.ext.ndb.Interestingly enough, currently there is an unused class property
PolyModel._class_mapwhich is populated by it but is never read. On the other side we havemodel._entity_from_ds_entitythat discards the rest of theclass_keypassed to it, taking just just the last element and looking it as if it were an ordinary model. These two shortcomings are as if calling for each other - an unused property and a client using incomplete data.This PR fixes this inconsistency and re-enables the ability to read and write polymodels with equal names if their class_keys are different. For this purpose the
_class_mapproperty is moved toModelclass so that it can read from it. I avoided introduction of a function for accessing it, because unlikelookup_kindthis function should not raise an exception if the model is not found. Anyhow, it is requested during the review, I will add a wrapper function along with its tests or perform any other changes more persistent to the design of the library.